Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add labels meta #959

Merged
merged 5 commits into from
Sep 23, 2024
Merged

feat: add labels meta #959

merged 5 commits into from
Sep 23, 2024

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Sep 19, 2024 •
edited
Loading

Allows the ability to add meta via fields.* fields, data will be appended under labels.* in heartbeat documents !!

fixes #729 !!

This will only work with kibana 8.16.0 with PR elastic/kibana#193250 !!


  monitor.use({
    id: 'example-monitor',
    schedule: 10,
    fields: {
      foo: 'bar',
      team: 'synthetics',
    }
  });

lightweight


heartbeat.monitors:
- type: http
  name: Todos Lightweight
  id: "todos- light  weightt
  enabled: true
  urls: "https://elastic.github.io/synthetics-demo"
  schedule: '@every 3m10s'
  timeout: 16s
  alert.status.enabled: true
  fields.environment: "production"
  fields.team: "todos"

@emilioalvap
Copy link
Contributor

@shahzad31 I'm worried this will confusing since the lightweight representation is not a valid heartbeat monitor and effectively undocumented, wdyt?

@shahzad31
Copy link
Contributor Author

shahzad31 commented Sep 19, 2024 •
edited
Loading

@shahzad31 I'm worried this will confusing since the lightweight representation is not a valid heartbeat monitor and effectively undocumented, wdyt?

@emilioalvap i will create a docs issue in a follow up to document this.

@shahzad31 shahzad31 mentioned this pull request Sep 19, 2024
Closed
emilioalvap
emilioalvap reviewed Sep 19, 2024
View reviewed changes
src/push/monitor.ts Outdated
@@ -268,6 +268,7 @@ export function buildMonitorFromYaml(
enabled: config.enabled ?? options.enabled,
locations: options.locations,
tags: options.tags,
labels: parseLabels(config, options.labels),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this new field to some of the unit tests validating lightweight monitors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have added here 200e52f (#959)

vigneshshanmugam
vigneshshanmugam reviewed Sep 19, 2024
View reviewed changes
src/common_types.ts Outdated
@@ -255,6 +255,7 @@ export type PushOptions = Partial<ProjectSettings> &
kibanaVersion?: string;
yes?: boolean;
tags?: Array<string>;
labels?: Record<string, string>;
Copy link
Member

@vigneshshanmugam vigneshshanmugam Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to fields to map with the heartbeat top level fields name and under the hood in Kibana, you can map to labels. Having labels as top level is confusing - https://www.elastic.co/guide/en/beats/heartbeat/8.12/configuration-general-options.html#libbeat-configuration-fields

Also let's write JS doc annotation so it's visible on TS config.

vigneshshanmugam
vigneshshanmugam reviewed Sep 19, 2024
View reviewed changes
__tests__/push/monitor.test.ts Outdated
@@ -472,6 +472,50 @@ heartbeat.monitors:
hosts: ['elastic.co:443'],
});
});

it('supports labels in config', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add test for browser monitor as well.

@paulb-elastic
Copy link
Contributor

@shahzad31

This will only work with kibana 8.16.0

If using < 8.16.0, are these just ignored (i.e. it won't fail to push)?

@shahzad31
Copy link
Contributor Author

@shahzad31

This will only work with kibana 8.16.0

If using < 8.16.0, are these just ignored (i.e. it won't fail to push)?

@paulb-elastic validation will fail the push. User will need to remove labels.

vigneshshanmugam
vigneshshanmugam reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, Can you add a test for Browser monitor and supplying both global and monitor fields. Thanks

__tests__/push/monitor.test.ts Outdated Show resolved Hide resolved
src/common_types.ts Outdated Show resolved Hide resolved
vigneshshanmugam
vigneshshanmugam approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vigneshshanmugam vigneshshanmugam merged commit b6bfbeb into main Sep 23, 2024
8 checks passed
@vigneshshanmugam vigneshshanmugam deleted the add-labels branch September 23, 2024 16:45
@emilioalvap emilioalvap mentioned this pull request Oct 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilioalvap emilioalvap emilioalvap left review comments

@vigneshshanmugam vigneshshanmugam vigneshshanmugam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Synthetics] - add meta information to monitoring data
4 participants
@shahzad31 @emilioalvap @paulb-elastic @vigneshshanmugam